Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hadolint errors #1048

Merged
merged 9 commits into from
Mar 8, 2023
Merged

Fix hadolint errors #1048

merged 9 commits into from
Mar 8, 2023

Conversation

NotMyFault
Copy link
Member

#1043 didn't run on infra.ci, therefore hadolint errors remained invisible.
This PR restores deployment if the build is green. Additionally, let's see if infra.ci picks up this PR.

@NotMyFault NotMyFault force-pushed the fix-hadolint branch 2 times, most recently from 086b453 to 9e3c81c Compare March 6, 2023 11:54
@NotMyFault NotMyFault force-pushed the fix-hadolint branch 2 times, most recently from 4828691 to 2a21d13 Compare March 6, 2023 12:37
raul-arabaolaza added a commit to raul-arabaolaza/acceptance-test-harness that referenced this pull request Mar 7, 2023
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
@NotMyFault NotMyFault requested a review from dduportal March 7, 2023 13:21
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
dduportal pushed a commit that referenced this pull request Mar 8, 2023
* [ATH-1036] Fix docker image so docker is available

Reverts c54ef5f#diff-59086a3e54e38095246aa3a051f9020274dbf005b4652a7808cd4c6c239c536eL67

* Empty-Commit to retrigger

* Update src/main/resources/ath-container/Dockerfile

Co-authored-by: James Nord <jtnord@users.noreply.github.com>

* [ATH-1036] Use oldstable to get viewvc

See refenrence in Dockerfile

* [ATH-1036] Fix atlassian-sdk installation

* Partially revert #1040

* [ATH-1036] Make sure the container contains the ssh client

* [ATH-1036] Partially revert #1048

* Empty-Commit to retrigger

* [ATH-1036] Do not use xenial for tomcat container

* [ATH-1036] Use tomcat 10 with ubuntu 22.04 for DeployPluginTest

As tomcat 7 is not java11 compatible

* [ATH-1036] Try to use ubuntu 22.04

* Empty-Commit to retrigger

* Ignore flaky test until issue 1052 is resolved

* Empty-Commit to retrigger

---------

Co-authored-by: James Nord <jtnord@users.noreply.github.com>
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
Comment on lines 86 to 90
# hadolint: Do not pin packages as they are requirements only for docker installation (and not of the rest of the ATH) and the effort required to track each version would be too huge
# hadolint ignore=DL3008
RUN apt-get update --quiet && apt-get install --yes --no-install-recommends docker-ce docker-ce-cli docker-buildx-plugin \
&& apt-get clean \
&& rm -rf /var/cache/apt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Docker CE version is pinned by the instruction above, which is a good thing (Docker actually have breaking changes some time to time: 22.x to 23.x did break a lot of things in a good way, but broke nevertheless).

But the "pinning" on APT is useful in a "long running instance" such as a VM. We can improve the current pinning by using pure-Docker form to make it easier to read and maintain.

My proposal is a refinment of the existing code and include removing the whole instruction above (RUN echo "..." | tee /etc/apt/preferences.d/docker-ce because I can't make a suggestion on unchanged code):

Suggested change
# hadolint: Do not pin packages as they are requirements only for docker installation (and not of the rest of the ATH) and the effort required to track each version would be too huge
# hadolint ignore=DL3008
RUN apt-get update --quiet && apt-get install --yes --no-install-recommends docker-ce docker-ce-cli docker-buildx-plugin \
&& apt-get clean \
&& rm -rf /var/cache/apt
# Ensure that version is fixed
ARG DOCKER_VERSION=23.0.1
ARG DOCKER_BUILDX_VERSION=0.10.2
RUN apt-get update --quiet \
&& apt-get install --yes --no-install-recommends \
docker-ce="5:${DOCKER_VERSION}*" \
docker-ce-cli="5:${DOCKER_VERSION}*" \
docker-buildx-plugin="${DOCKER_BUILDX_VERSION}*" \
&& apt-get clean \
&& rm -rf /var/cache/apt

=> no need to ignore package pinning (in this particular case, Docker-CE and Docker BuildX, pinning is important for the ATH usage otherwise it will break in 3 month with the next Docker CE update) in hadolint

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine, I wasn't aware of that many breaking changes here 👀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem, it was a good opportunity to point it out so the time you spent on fixing hadolint would be beneficial for you and the ATH

@NotMyFault NotMyFault enabled auto-merge (squash) March 8, 2023 13:40
@NotMyFault NotMyFault merged commit b10af89 into jenkinsci:master Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants